Skip to content

Java/C++/C#: Add nodes predicate to PathGraph.#1881

Merged
hvitved merged 5 commits into
github:masterfrom
aschackmull:java/pathgraph-nodes
Sep 13, 2019
Merged

Java/C++/C#: Add nodes predicate to PathGraph.#1881
hvitved merged 5 commits into
github:masterfrom
aschackmull:java/pathgraph-nodes

Conversation

@aschackmull

@aschackmull aschackmull commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

This ensures that we show the toString and thus the accesspath in all contexts, in particular lgtm.com.

@aschackmull aschackmull requested review from a team as code owners September 5, 2019 14:47
jbj
jbj previously approved these changes Sep 5, 2019
View reviewed changes
hvitved
hvitved reviewed Sep 5, 2019
View reviewed changes

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we should be able to get rid of the direct source-to-sink step (https://github.com/aschackmull/ql/blob/a898f5781e9bff1b5dce747d18a8c37ccb47aada/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll#L1662), right?

yh-semmle
yh-semmle suggested changes Sep 6, 2019
View reviewed changes

@yh-semmle yh-semmle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected test output still needs updating.

@aschackmull aschackmull mentioned this pull request Sep 12, 2019
Merged
@aschackmull

aschackmull commented Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

I've included a commit to fix the toString of an empty accesspath, which was a postponed change from #1797.

hvitved
hvitved requested changes Sep 12, 2019
View reviewed changes

@hvitved hvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above.

@aschackmull

aschackmull commented Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

This means we should be able to get rid of the direct source-to-sink step

Possibly, yes. But I think that's best left to a separate PR. This is about getting the right toString to lgtm.com.

@aschackmull aschackmull added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 12, 2019
@hvitved

hvitved commented Sep 12, 2019

Copy link
Copy Markdown
Contributor

Possibly, yes. But I think that's best left to a separate PR. This is about getting the right toString to lgtm.com.

OK, I would have thought the two would go together; the sink-to-source step was basically a workaround needed because of the missing nodes predicate.

@aschackmull

aschackmull commented Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

Language-Tests/Java is btw. not expected to succeed here, but it is green on the internal PR.

@hvitved hvitved merged commit f5cae9b into github:master Sep 13, 2019
@aschackmull aschackmull deleted the java/pathgraph-nodes branch September 13, 2019 08:34
@jbj jbj mentioned this pull request Sep 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hvitved hvitved hvitved approved these changes
@jbj jbj jbj left review comments
+1 more reviewer
@yh-semmle yh-semmle yh-semmle approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aschackmull @hvitved @jbj @yh-semmle